Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/ocrvs 7978/qr reader #8196

Open
wants to merge 62 commits into
base: develop
Choose a base branch
from
Open

Conversation

tahmidrahman-dsi
Copy link
Collaborator

@tahmidrahman-dsi tahmidrahman-dsi commented Dec 11, 2024

Reason for this feature

To support countries which implemented identity systems to obtain identifiers for its residents. A common use case is that there is a QR code associated with the resident's identity card provided by the identity system. The QR code holds the person information and can be used to pre-fill event form. We need to support this use case.

What this feature does

This feature introduces QR code scanning ability to the application via a UI component called <IDReader /> which can be used in form with type ID_READER.

Bundle size effects

Before (develop):

build/registerSW.js                 0.14 kB
build/manifest.webmanifest          0.15 kB
build/index.html                    3.39 kB │ gzip:     1.44 kB
build/assets/index-Bqb48nWC.js  7,243.64 kB │ gzip: 2,025.47 kB │ map: 23,854.29 kB

After (feature branch merged with develop)

build/registerSW.js                                 0.14 kB
build/manifest.webmanifest                          0.15 kB
build/index.html                                    3.39 kB │ gzip:     1.44 kB
build/assets/qr-scanner-worker.min-D85Z9gVD.js     44.01 kB │ gzip:    10.50 kB │ map:     45.01 kB
build/assets/index-BiU8_hQ6.js                  7,328.06 kB │ gzip: 2,040.90 kB │ map: 24,047.98 kB

Merging this ticket closes #7978, #7979

@tahmidrahman-dsi
Copy link
Collaborator Author

/uikit

@naftis naftis marked this pull request as ready for review December 12, 2024 08:09
@naftis
Copy link
Collaborator

naftis commented Dec 12, 2024

/uikit

@ocrvs-bot
Copy link
Collaborator

Storybook deployed: https://feat-ocrvs-7978-qr-reader.opencrvs.pages.dev

Comment on lines +24 to +43
display: flex;
align-items: center;
width: ${width || '100%'};

:before, :after {
flex: 1;
content: '';
padding: 0 1px 1px;
background-color: ${color || theme.colors.grey200};
margin: 16px;
}

:before {
margin-left: 0;
}

:after {
margin-right: 0;
}
`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically what does this do if Divider has children? Could this be documented in Storybook 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep sure! I introduced children prop to the divider to make components like this below. As it serves the same purpose of a divider component, I made the prop change in the divider component

image

Comment on lines 32 to 44
const renderReader = (reader: ReaderType) => {
const { type, ...readerProps } = reader
if (type === 'qr') {
return (
<QRReader
key={type}
{...readerProps}
onScan={onScan}
onError={onError}
/>
)
} else return null
}
Copy link
Collaborator

@naftis naftis Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a "ReaderGenerator" (like FormFieldGenerator), can be lifted to a separate component so it doesn't get re-created on every render of IDReader

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I am on it 👍🏼

<Icon name={props.iconName} size="large" />
</IconContainer>
<Text variant="reg14" element="p">
{props.label}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{props.label}
{children}

Comment on lines 23 to 25
const StyledButton = styled(SecondaryButton)`
width: 100%;
`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Button component instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thanks. will do

@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 7, 2025 06:50 — with GitHub Actions Inactive
@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 7, 2025 09:51 — with GitHub Actions Inactive
@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 9, 2025 07:04 — with GitHub Actions Inactive
@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 9, 2025 10:13 — with GitHub Actions Inactive
@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 9, 2025 10:36 — with GitHub Actions Inactive
@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 9, 2025 11:02 — with GitHub Actions Inactive
@tahmidrahman-dsi
Copy link
Collaborator Author

tahmidrahman-dsi commented Jan 9, 2025

@euanmillar @naftis now link button will use both the params defined in form config and those came after the redirection when calling the callback URL

For instance, for a callback defined below

{
    name: fieldName,
    validator: [],
    icon: {
      desktop: 'Globe',
      mobile: 'Fingerprint'
    },
    type: 'LINK_BUTTON',
    custom: true,
    label: {
      id: 'views.idReader.label.eSignet',
      defaultMessage: 'E-signet'
    },
    hideInPreview: true,
    conditionals: [
      {
        action: 'disable',
        expression: '!!$form.redirectCallbackFetch'
      }
    ],
    options: {
      url: esignetAuthUrl,
      callback: {
        params: {
          // code: 'esignet-mock-code'  // We can skip defining code and let the redirection view implicitly forward
          state: 'fetch-on-mount'  // required to trigger callback fetch
        },
        trigger: callbackFieldName
      }
    }
}
{
  name: fieldName,
  type: 'HTTP',
  custom: true,
  label: {
    id: 'form.field.label.empty',
    defaultMessage: ''
  },
  validator: [],
  options: {
    getOIDPUserInfoUrl,
    headers: {
      'Content-type': 'application/json'
    },
    body: {
      // code: 'esignet-mock-code', // as the code is sent with the params, no need to pass with the request body
      clientId: openIdProviderClientId,
      redirectUri: ''"
    },
    method: 'POST'
  }
}

After the redirection url sends back a code when to opencrvs form
{{CLIENT_URL}}/drafts/:draftId/events/birth/child/group/child-view-group?code=<some-auth-code>&state=fetch-on-mount

When the form is rendered with the link button, the link button looks in the param state with value fetch-on-mount and then it triggers the callback with below url

<oidp-user-info-url>?code=<some-auth-code>&state=fetch-on-mount

Please let me know if there is any improvement needed

@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 9, 2025 13:32 — with GitHub Actions Inactive
@tahmidrahman-dsi tahmidrahman-dsi linked an issue Jan 10, 2025 that may be closed by this pull request
1 task
@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 10, 2025 13:07 — with GitHub Actions Inactive
@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 14, 2025 07:13 — with GitHub Actions Inactive
@naftis naftis self-requested a review January 14, 2025 08:18
Comment on lines +72 to +87
function checkParamsPresentInURL() {
for (const [key, value] of Object.entries(params)) {
if (urlParams.get(key) !== value) {
return false
}
}
return true
}
if (checkParamsPresentInURL() && !hasCallbackRequestBeenMade) {
call({
// forward params which are received after redirection to the callback request
params: Object.fromEntries(urlParams)
})
setCallbackRequestBeenMade(true)
}
}, [call, params, form, trigger, hasCallbackRequestBeenMade])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tahmidrahman-dsi how / where is this call/params functionality documented? It's a good method, but can be confusing for someone wanting to use the component.

Comment on lines +743 to +747
/**
* If the redirection url has the exact same param keys
* with exact same values sepecified in the below `params`
* field, only then the callback will be triggered
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const Scanner = (props: ScannerProps) => {
const scanner = useRef<QRScanner>()
const videoElement = useRef<HTMLVideoElement>(null)
const [qrOn, setQrOn] = useState(true)
Copy link
Collaborator

@naftis naftis Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read this correctly, qrOn can never be false. When the camera cannot be used, we need to consider the error cases a bit. Let's create a new ticket on that and solve it later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks @naftis that is true and because of the fact that qrOn is never false,


this line becomes irrelevant. We might need to determine how to handle that case as the current way is showing an alert which was an interim change and not sure if it should stay like this

Issue created: #8330

@tahmidrahman-dsi tahmidrahman-dsi temporarily deployed to featocrvs-7978qr-reader January 16, 2025 11:19 — with GitHub Actions Inactive
@tahmidrahman-dsi tahmidrahman-dsi deployed to featocrvs-7978qr-reader January 16, 2025 13:20 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 Ready to deploy Deployment automation should pick this PR up and start auto-deploying it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QR Code scanner form field QR Code scanner UI component
3 participants